-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add: Manage grafana content/assets via terraform #913
base: main
Are you sure you want to change the base?
✨ Add: Manage grafana content/assets via terraform #913
Conversation
@@ -0,0 +1 @@ | |||
1.5.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the right place for the .terraform-version file? Should we rather have only one tf version for the whole repo, at the repo base dir, and maybe even have a pre-commit hook that enforces only one single tf version? not sure
url = var.prometheus_catchall_url | ||
basic_auth_enabled = false | ||
is_default = false | ||
uid = "RmZEr52nz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "could" have been a password (the academic proper term is "high entropy string" I guess). I think we should have some sort of test (pre-commit?) that asserts that no high-entropy strings are commited in this repo, as it is public on github, and we might leak secrets, and rotating secrets is a lot of annoying work, so I'd like that not to happen :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
I wonder what can be done with ci.env
file
# Extract DEPLOYMENT_FQDN using Make functions | ||
DEPLOYMENT_FQDN := $(notdir $(patsubst %/,%, $(dir $(REPO_CONFIG_LOCATION)))) | ||
# Construct CI_ENV_FILE using Make functions | ||
CI_ENV_FILE := $(realpath $(dir $(REPO_CONFIG_LOCATION))/../../.gitlab/pipelines/1_configurations/$(DEPLOYMENT_FQDN)/ci.env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if hardcoding assumptions about the other repo here is sustainable.
We can brainstorm together what can be done and if we can avoid using ci.env
file here
DEPLOYMENT_FQDN := $(notdir $(patsubst %/,%, $(dir $(REPO_CONFIG_LOCATION)))) | ||
# Construct CI_ENV_FILE using Make functions | ||
CI_ENV_FILE := $(realpath $(dir $(REPO_CONFIG_LOCATION))/../../.gitlab/pipelines/1_configurations/$(DEPLOYMENT_FQDN)/ci.env) | ||
$(if $(CI_ENV_FILE),,$(error The location of the repo.config file given in .config.location is invalid. Cannot find the ci.env file. Aborting)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not going to work in CI where we do not have ci.env
file
@@ -253,13 +260,13 @@ venv: $(REPO_BASE_DIR)/.venv/bin/activate ## Creates a python virtual environmen | |||
ifeq ($(shell test -f j2cli_customization.py && echo -n yes),yes) | |||
|
|||
define jinja | |||
$(REPO_BASE_DIR)/.venv/bin/j2 --format=env $(1) .env -o $(2) --customize j2cli_customization.py | |||
$(REPO_BASE_DIR)/.venv/bin/j2 --format=env $(1) $(2) -o $(3) --customize j2cli_customization.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if for a single corner case we need to repeat .env
call in all places except the corner case
Ideas:
- use default value
.env
if only 2 arguments are passed - define special / different
jinja
function for grafana terraform
$(REPO_BASE_DIR)/.venv/bin/j2 --format=env $(1) $(2) -o $(3) --customize j2cli_customization.py | |
$(REPO_BASE_DIR)/.venv/bin/j2 --format=env $(1) $(if $(strip $(3)),$(2),.env) -o $(if $(strip $(3)),$(3),$(2)) --customize j2cli_customization.py |
set +o allexport; \ | ||
url=$${TF_VAR_GRAFANA_URL}; \ | ||
echo "Waiting for grafana at $$url to become reachable..."; \ | ||
while true; do \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we add a timeout here to avoid waiting forever?
@@ -0,0 +1,35 @@ | |||
// Import subfolders using an external script | |||
data "external" "subfolders" { | |||
program = ["bash", "${path.module}/../../../../scripts/tf_helper_list_subfolders.sh", "${path.module}/../assets/shared/dashboards"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q
since grafana dashboards are now controlled by terraform, shall we store them directly in terraform? Thus we can also avoid any relative ../
imports
data "external" "dashboard_files" { | ||
for_each = local.folder_map | ||
|
||
program = ["bash", "${path.module}/../../../../scripts/tf_helper_list_json_files_in_folder.sh", "${path.module}/../assets/shared/dashboards/${each.key}"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can keep this scripts inside terraform folder to avoid having ../../../../
terraform/main.tf: terraform/main.tf.j2 .venv $(CI_ENV_FILE) | ||
# generate $@ | ||
@$(call jinja, $<, $(CI_ENV_FILE), $@) | ||
# validate and format $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it a redundant line?
@@ -0,0 +1,31 @@ | |||
terraform { | |||
required_version = "~> 1.5.1" | |||
{% if TF_STATE_BACKEND_TYPE != "s3" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stricter check? there are many terraform backend types
@@ -0,0 +1,6 @@ | |||
main.tf | |||
.terraform/** */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: this */
is to ignore all files but it does not embrace hidden files, correct?
What do these changes do?
CI_ENV_FILE
tocommon.Makefile
(necessary for handling S3 tf-state in grafana-tf)grafana-export
Makefile target, old python grafana import/export scriptsRelated issue/s
#778
Related PR/s
https://git.speag.com/oSparc/osparc-ops-deployment-configuration/-/merge_requests/1170
Checklist